Skip to content

PI: Index named destinations when appending a document - #4360

Open
kholdrex wants to merge 7 commits into
py-pdf:mainfrom
kholdrex:named-destinations-index
Open

kholdrex wants to merge 7 commits into
py-pdf:mainfrom
kholdrex:named-destinations-index

Conversation

@kholdrex

@kholdrex kholdrex commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

merge() adds the named destinations of the appended document one at a time. For each one it checks title in names and then add_named_destination_array() scans the array again to find where to insert, so a document with many named destinations takes quadratic time.

For documents with at least 32 named destinations, the string entries now go into a set for the lookup. If the existing keys are sorted, the new destinations are collected and sorted into the array once at the end. Putting each one before the first greater key, as add_named_destination_array() does, gives the same order as that stable sort. Arrays with unsorted keys still use the old insertion, and documents with fewer destinations keep the old code, since building the index reads the whole array.

writer.append(reader) with all destinations on one page:

Named destinations main this PR
2,000 0.24 s 0.04 s
4,000 0.88 s 0.07 s
8,000 3.3 s 0.19 s

The resulting /Names arrays and written files are the same as on main in about 1,300 generated cases (unsorted and odd-length arrays, indirect arrays, byte string keys, clone_from writers, appending the same file twice), and appending many small documents into a large array is as fast as before.

AI assistance: Claude Code (Claude Opus 5.5) helped write and measure this, and Codex (GPT-6 Astra) reviewed it. I reviewed the change and ran the test suite.

@codecov

codecov Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.88%. Comparing base (1e3e7e3) to head (654f603).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4360   +/-   ##
=======================================
  Coverage   98.87%   98.88%           
=======================================
  Files          59       59           
  Lines       12299    12357   +58     
  Branches     2342     2350    +8     
=======================================
+ Hits        12161    12219   +58     
  Misses         68       68           
  Partials       70       70           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread pypdf/_writer.py Outdated
self._is_sorted = False
self._pending: list[tuple[str, Any]] = []

def _update(self, array: list[Any]) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we able to avoid the name array here, as this is a stdlib module as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed it to names.

Comment thread pypdf/_writer.py
_NAMED_DESTINATION_INDEX_MINIMUM = 32


class _NamedDestinationIndex:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be any benefits to subclass list or similar here and re-use the common API?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would help much: the index doesn't own the /Names array, the ArrayObject stays in the document and the index only sits next to it during one merge(). A list subclass would mean swapping that object in the document, while add_named_destination_array() and get_named_dest_root() keep working on the original. Happy to change it if you see a cleaner way.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants